Skip to content

Conversation

@jfreden
Copy link
Contributor

@jfreden jfreden commented Oct 29, 2024

This PR adds a cleanup security migration for role mapping duplicates that exist both in cluster state and the .security index. The cleanup task is implemented as a security migration.

The task compares the names of the role mappings in the operator-defined settings.json and the names of the role mappings in the .security index and removes the ones in the .security index.

See related PR for full review: #114830

Issue

This cleanup is done because there was a bug (fixed #114337) that caused ECK customers to have the same role mappings with the same names present both in cluster state and the .security index. The effective role mapping is the combination of the .security index mapping and the cluster state one. After the bug #114337, if the mapping in cluster state is modified (through setting.json), the .security index will still contain the old mapping and this could lead to unintended behaviour.

@elasticsearchmachine elasticsearchmachine added Team:Security Meta label for security team v9.0.0 labels Oct 29, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

Hi @jfreden, I've created a changelog YAML for you.

Copy link
Contributor

@n1v0lg n1v0lg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 Great work on this, and thanks for iterating!

// i.e. it is NOT created in a previous ES version that potentially didn't index the role metadata
// * or, the .security index has been migrated (using an internal update-by-query) such that the metadata is queryable
return frozenSecurityIndex.isCreatedOnLatestVersion()
return frozenSecurityIndex.isCreatedOnLatestMigrationVersion()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets rename this back to isCreatedOnLatestVersion

"metadata.indices"
);
assertNotNull(indices);
// JsonMapView doesn't support . prefixed indices (splits on .)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that indices contain a key for .security-7?

@jfreden jfreden added v8.17.0 and removed backport labels Oct 29, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @jfreden, I've created a changelog YAML for you.

@jfreden jfreden added the auto-backport Automatically create backport pull requests when merged label Oct 29, 2024
@jfreden jfreden merged commit a7031d8 into elastic:main Oct 29, 2024
20 of 21 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 115823

jfreden added a commit to jfreden/elasticsearch that referenced this pull request Oct 29, 2024
* Add security migration for cleaning up ECK role mappings

(cherry picked from commit a7031d8)

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/IndexVersions.java
#	x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java
#	x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/AbstractUpgradeTestCase.java
@jfreden
Copy link
Contributor Author

jfreden commented Oct 29, 2024

💔 Some backports could not be created

Status Branch Result
8.x Conflict resolution was aborted by the user
8.16

Manual backport

To create the backport manually run:

backport --pr 115823

Questions ?

Please refer to the Backport tool documentation

jfreden added a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
* Add security migration for cleaning up ECK role mappings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged >bug :Security/Security Security issues without another label Team:Security Meta label for security team v8.16.0 v8.17.0 v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants